Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: tickLabelProps dy and dx work again #271

Merged
merged 2 commits into from
Apr 19, 2018
Merged

Conversation

sto3psl
Copy link
Contributor

@sto3psl sto3psl commented Apr 16, 2018

This should fix the bug mentioned in #267.
I had several problems with this which made me come to the solution of using transforms for this bug fix.

  1. px values for dx and dy were fine and worked.
  2. em values were fine as long as fontSize was explicitly provided for <Text />. Without fontSize there is no value to use for the transformation from em -> px without accessing the DOM.
  3. rem units had the same problem that there is no way of transforming to px without the DOM.
  4. I couldn't do <text dy="1em">...</text> because the child <tspan> defines a dy value that overwrites the one from <text>. I'm not quite sure why. example

In the end setting a CSS transform seemed like the easiest solution and has the nice benefit of supporting rem.

If there is a better solution please let me know, this took longer than I want to admit 😅.

🐛 Bug Fix

  • @vx/text support dx and dy again

@hshoff
Copy link
Member

hshoff commented Apr 17, 2018

Clever solution! My only thought is how this might play with rotating the text. Will give this branch a go locally when I get a chance and report back

@sto3psl
Copy link
Contributor Author

sto3psl commented Apr 18, 2018

screen shot 2018-04-18 at 12 04 27

In the screenshot I set angle: -45.

I checked myself and it did not work with rotation. My new commit fixes it. Let me know if there are any problems.

@techniq
Copy link
Collaborator

techniq commented Apr 18, 2018

Does this work on all (our supported) browsers? I remember reading that css transforms did not work on IE. It would be nice to support IE 11 if possible, although I know it's on it's way out (github is dropping it in a few months).

@sto3psl
Copy link
Contributor Author

sto3psl commented Apr 18, 2018

From caniuse about IE11.

Does not support CSS transforms on SVG elements (transform attribute can be used instead)

All versions of Edge have the same problem unfortunately.

I can change it to use the transform attribute which means that em and rem don't work that nice anymore and would need to be calculated.

@sto3psl
Copy link
Contributor Author

sto3psl commented Apr 18, 2018

The problem of transforming em -> px is only there if no font-size is set on <Text /> (or stuff needs to go into the DOM which is not so nice).

Would it be a good tradeoff to set a default fontSize prop of maybe 11px that there is always a value to calculate dx and dy with?

That would get rid of the transforms, uses the correct svg attributes and works in supported browsers.

@techniq
Copy link
Collaborator

techniq commented Apr 18, 2018

@sto3psl Since adding dx/dy on <text> conflicts with the <tspan>, maybe we could wrap the entire thing in a <g> and apply dx/dy to x/y?

@techniq
Copy link
Collaborator

techniq commented Apr 18, 2018

Another thought is could you use reduce-css-calc like we are for verticalAnchor and convert it that way

change:

const x = textProps.x + dx;
const y = textProps.y + dy;

to

const x = reduceCSSCalc(`calc(${textProps.x}) + ${dx}`);
const y = reduceCSSCalc(`calc(${textProps.y}) + ${dy}`);

@sto3psl
Copy link
Contributor Author

sto3psl commented Apr 18, 2018

Wrapping <text /> in a <g /> could work.

The problem with reduceCSSCalc is if dx or dy are em values and textProps.x or textProps.y are px values.

reduceCSSCalc(`calc(5px + 1em)`) -> 'calc(5px + 1em)'

And 'calc(5px + 1em)' is not valid as x or y value. reduceCSSCalc can't handle mixed units.

@techniq
Copy link
Collaborator

techniq commented Apr 18, 2018

@sto3psl oh, I thought it handled mixed units, but I see it does not. Hopefully the <g> route works. Thanks for digging into the problem.

@sto3psl
Copy link
Contributor Author

sto3psl commented Apr 18, 2018

Ok I played with svg a little and <g> does not work since it ignores x and y. We could wrap the <text> with a <svg> though and then we get px,em,rem support. According to the spec it's no problem if svg elements have svgs as child. See this codepen -> https://codepen.io/anon/pen/rvNGjz

@techniq
Copy link
Collaborator

techniq commented Apr 18, 2018

Our Group uses <g transform={`translate(${x}), ${y})`} />. Maybe try using the svg transform method on g? You could also use the <Group> component to simplify it? I'm assuming svg transforms understand em/rem (like css transform).

If this doesn't work, the svg might be a decent approach (would be odd to have a lot of nested SVG components, not sure if this could have any corner case problems).

@sto3psl
Copy link
Contributor Author

sto3psl commented Apr 18, 2018

Svg transforms are unitless which means transforming em -> px again. The <svg> approach is really nice and Victory does a lot <svg> nesting too.

@techniq
Copy link
Collaborator

techniq commented Apr 18, 2018

@sto3psl sounds like <svg> is the way to go then 😄. Thanks again

@sto3psl
Copy link
Contributor Author

sto3psl commented Apr 18, 2018

The <svg> seems to work nicely.

  • The style={{ overflow: "visible" }} is necessary since the svg has no width and height and the text would otherwise not be visible.
  • The fontSize has to be set so that the browser calculates em values correctly.

Thanks for all the input and thoughts on this @techniq. If there is anything else let me know, this was quite fun to think about 😄.

@techniq
Copy link
Collaborator

techniq commented Apr 18, 2018

@sto3psl Thanks for digging in and finding a solution. I haven't had a chance to pull it down locally (@hshoff if you could take a quick peak that would be awesome) but I have no reason to expect any issues 😄

@hshoff hshoff added this to the v0.0.161 milestone Apr 19, 2018
@hshoff
Copy link
Member

hshoff commented Apr 19, 2018

LGTM thanks for the contribution @sto3psl!

@hshoff hshoff merged commit 772ccea into airbnb:master Apr 19, 2018
@sto3psl sto3psl deleted the text-em branch September 10, 2018 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants